Open
Conversation
|
Check out this pull request on See visual diffs & provide feedback on Jupyter Notebooks. Powered by ReviewNB |
📝 WalkthroughWalkthroughUpdated dependency sourcing and migrated the band_gap notebook to new API patterns: clusters retrieved via Changes
Sequence Diagram(s)mermaid Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
utils/visualize.py (1)
406-408: Filter logic is correct; consider an early return when all results are filtered out.The
"value" in r or "values" in rcheck is the right way to guard displayable entries. One minor usability gap: if every item is stripped by the filter, the function proceeds silently —renderResultsreceives[]with no user-visible feedback.💡 Optional early-return guard
results = [r for r in results if "value" in r or "values" in r] + + if not results: + print("No displayable properties found in results.") + return🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@utils/visualize.py` around lines 406 - 408, After filtering results with results = [r for r in results if "value" in r or "values" in r], add an early-return guard that detects an empty results list and surfaces a user-visible message instead of passing [] to renderResults; e.g., if not results: call renderResults with a succinct "No displayable results" message (or invoke an existing no-results helper) and return from the current function so downstream code (and renderResults) aren't invoked with an empty list.pyproject.toml (1)
12-19: Both git dependencies are pinned to anonymous commit hashes — prefer named tags.
mat3ra-api-client@141d7dba...andmat3ra-prode@5b2e02dd...are bare SHA pins with no corresponding tag reference. This makes it impossible to tell at a glance what feature or release the pin corresponds to, and makes future bump PRs harder to review. Pin to a release tag (or at minimum add an inline comment with the version/date) once tags are available on those repos.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pyproject.toml` around lines 12 - 19, The two git dependencies in pyproject.toml are pinned to raw commit SHAs ("mat3ra-api-client @ git+https://github.com/Exabyte-io/api-client.git@141d7dba220f07844e97557dda3ee74087b38c31" and "mat3ra-prode @ git+https://github.com/Exabyte-io/prode.git@5b2e02ddbaf5dd473f0acaf1e1d76ddadc6bb184"); replace each SHA with the corresponding release tag (or branch name) from those repositories (e.g., vX.Y.Z) so the dependency shows a human-readable version, or if tags are not yet available add a short inline comment after the dependency naming the intended version/date and why the SHA is used; update the two entries "mat3ra-api-client" and "mat3ra-prode" accordingly and run your dependency installer to verify resolution.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@other/materials_designer/workflows/band_gap.ipynb`:
- Around line 517-526: The code assumes at least one cluster exists and directly
indexes clusters[0], which raises IndexError when client.clusters.list() returns
an empty list; add a guard that checks the clusters list (e.g., if not clusters
or len(clusters) == 0) before using clusters[0], and handle the empty case by
raising a clear exception or logging an error and aborting/using a fallback
cluster; update the block around the clusters variable and the Compute(...)
instantiation (Compute, cluster, QueueName.D, ppn) to only construct Compute
when a valid cluster is present.
- Around line 103-115: Remove the hardcoded os.environ assignments
(os.environ["API_HOST"], ["API_PORT"], ["API_SECURE"]) and the unused
API_HOST/API_PORT/API_SECURE/API_VERSION variables from the notebook cell;
instead either delete the cell or replace it with a short opt-in snippet or
commented example showing how to set local overrides (e.g., guarded by a boolean
flag like use_local = False or instructions to export env vars externally)
because APIClient.authenticate() reads from environment variables and these
assignments currently force all runs to localhost:3000.
In `@pyproject.toml`:
- Around line 11-12: The comment above the dependency line is incorrect — it
mentions "file URIs" and the three-slash rule, but the dependency
"mat3ra-api-client @
git+https://github.com/Exabyte-io/api-client.git@141d7dba220f07844e97557dda3ee74087b38c31"
is a VCS/git URL; update the comment to accurately describe that this is a git
VCS dependency using a commit SHA via the git+https://...@<commit> format (or
similar VCS URL guidance), and remove any reference to file URI syntax so the
comment correctly reflects the format used by the dependency line.
---
Nitpick comments:
In `@pyproject.toml`:
- Around line 12-19: The two git dependencies in pyproject.toml are pinned to
raw commit SHAs ("mat3ra-api-client @
git+https://github.com/Exabyte-io/api-client.git@141d7dba220f07844e97557dda3ee74087b38c31"
and "mat3ra-prode @
git+https://github.com/Exabyte-io/prode.git@5b2e02ddbaf5dd473f0acaf1e1d76ddadc6bb184");
replace each SHA with the corresponding release tag (or branch name) from those
repositories (e.g., vX.Y.Z) so the dependency shows a human-readable version, or
if tags are not yet available add a short inline comment after the dependency
naming the intended version/date and why the SHA is used; update the two entries
"mat3ra-api-client" and "mat3ra-prode" accordingly and run your dependency
installer to verify resolution.
In `@utils/visualize.py`:
- Around line 406-408: After filtering results with results = [r for r in
results if "value" in r or "values" in r], add an early-return guard that
detects an empty results list and surfaces a user-visible message instead of
passing [] to renderResults; e.g., if not results: call renderResults with a
succinct "No displayable results" message (or invoke an existing no-results
helper) and return from the current function so downstream code (and
renderResults) aren't invoked with an empty list.
Comment on lines
517
to
526
| "from mat3ra.ide.compute import Compute, QueueName\n", | ||
| "\n", | ||
| "CLUSTER_NAME = get_cluster_name()\n", | ||
| "cluster = clusters[0]\n", | ||
| "\n", | ||
| "compute = client.jobs.build_compute_config(\n", | ||
| " cluster=CLUSTER_NAME\n", | ||
| "compute = Compute(\n", | ||
| " cluster=cluster,\n", | ||
| " queue=QueueName.D,\n", | ||
| " ppn=2\n", | ||
| ")" | ||
| ] |
There was a problem hiding this comment.
clusters[0] raises IndexError when no clusters are available.
client.clusters.list() could return an empty list (no clusters provisioned for the account, API error, etc.), causing an unhandled IndexError on the next line.
🛡️ Proposed guard
cluster = clusters[0]
+if not clusters:
+ raise RuntimeError("No clusters available for this account. Please provision a cluster first.")
+cluster = clusters[0]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@other/materials_designer/workflows/band_gap.ipynb` around lines 517 - 526,
The code assumes at least one cluster exists and directly indexes clusters[0],
which raises IndexError when client.clusters.list() returns an empty list; add a
guard that checks the clusters list (e.g., if not clusters or len(clusters) ==
0) before using clusters[0], and handle the empty case by raising a clear
exception or logging an error and aborting/using a fallback cluster; update the
block around the clusters variable and the Compute(...) instantiation (Compute,
cluster, QueueName.D, ppn) to only construct Compute when a valid cluster is
present.
| @@ -465,7 +465,8 @@ | |||
| "id": "34", | |||
Member
There was a problem hiding this comment.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
other/materials_designer/workflows/band_gap.ipynb (1)
501-505: Remove explicitppnparameter to use the default value.The
Computeclass has a defaultppnvalue of 1. The explicitppn=2parameter can be removed to rely on the default, simplifying the code.♻️ Proposed fix
compute = Compute( cluster=cluster, queue=QueueName.D, - ppn=2 )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@other/materials_designer/workflows/band_gap.ipynb` around lines 501 - 505, The code explicitly sets ppn=2 when constructing Compute (Compute(cluster=cluster, queue=QueueName.D, ppn=2)); remove the ppn argument so the constructor relies on the class default (ppn=1). Update the Compute instantiation to omit the ppn parameter and leave cluster and queue unchanged, e.g., use Compute(cluster=cluster, queue=QueueName.D).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@other/materials_designer/workflows/band_gap.ipynb`:
- Around line 501-505: The code explicitly sets ppn=2 when constructing Compute
(Compute(cluster=cluster, queue=QueueName.D, ppn=2)); remove the ppn argument so
the constructor relies on the class default (ppn=1). Update the Compute
instantiation to omit the ppn parameter and leave cluster and queue unchanged,
e.g., use Compute(cluster=cluster, queue=QueueName.D).
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
config.ymlother/materials_designer/workflows/band_gap.ipynbpyproject.toml
🚧 Files skipped from review as they are similar to previous changes (2)
- config.yml
- pyproject.toml
There was a problem hiding this comment.
♻️ Duplicate comments (1)
other/materials_designer/workflows/band_gap.ipynb (1)
498-505:⚠️ Potential issue | 🟡 MinorGuard against empty clusters list.
clusters[0]will raiseIndexErrorifclient.clusters.list()returns an empty list (no clusters provisioned, API error, etc.). This concern was flagged in a previous review and remains unaddressed.🛡️ Proposed guard
from mat3ra.ide.compute import Compute, QueueName +if not clusters: + raise RuntimeError("No clusters available for this account. Please provision a cluster first.") cluster = clusters[0] compute = Compute( cluster=cluster, queue=QueueName.D, ppn=2 )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@other/materials_designer/workflows/band_gap.ipynb` around lines 498 - 505, The code assumes clusters[0] exists and will IndexError if the clusters list is empty; before selecting cluster for Compute (the variable cluster and class Compute/QueueName), check that clusters is non-empty (e.g., if not clusters: handle error or raise a clear exception or log and exit) and only then assign cluster = clusters[0] and construct Compute; ensure the error path provides a clear message about no available clusters so callers/debugging can proceed safely.
🧹 Nitpick comments (1)
other/materials_designer/workflows/band_gap.ipynb (1)
501-505: Consider removing hardcodedppn=2per reviewer feedback.A previous reviewer requested keeping the default value for
ppn. IfComputehas a sensible default, omittingppnwould make this example more portable across different cluster configurations.♻️ Proposed change
compute = Compute( cluster=cluster, queue=QueueName.D, - ppn=2 )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@other/materials_designer/workflows/band_gap.ipynb` around lines 501 - 505, The snippet hardcodes ppn=2 when constructing Compute; remove the ppn argument so Compute(cluster=cluster, queue=QueueName.D) relies on its internal/default ppn value; update the call site where Compute(...) is created (refer to the Compute constructor usage with cluster and QueueName.D) and ensure no other code assumes ppn was explicitly set.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@other/materials_designer/workflows/band_gap.ipynb`:
- Around line 498-505: The code assumes clusters[0] exists and will IndexError
if the clusters list is empty; before selecting cluster for Compute (the
variable cluster and class Compute/QueueName), check that clusters is non-empty
(e.g., if not clusters: handle error or raise a clear exception or log and exit)
and only then assign cluster = clusters[0] and construct Compute; ensure the
error path provides a clear message about no available clusters so
callers/debugging can proceed safely.
---
Nitpick comments:
In `@other/materials_designer/workflows/band_gap.ipynb`:
- Around line 501-505: The snippet hardcodes ppn=2 when constructing Compute;
remove the ppn argument so Compute(cluster=cluster, queue=QueueName.D) relies on
its internal/default ppn value; update the call site where Compute(...) is
created (refer to the Compute constructor usage with cluster and QueueName.D)
and ensure no other code assumes ppn was explicitly set.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Bug Fixes
Chores